Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/new new chips #344

Closed

Conversation

mansona
Copy link
Collaborator

@mansona mansona commented Apr 22, 2016

Second update of the Chips functionality. Continuation of #267

@mansona mansona mentioned this pull request Apr 22, 2016
@mansona
Copy link
Collaborator Author

mansona commented Apr 22, 2016

Howdy folks! It's me again 😄 I would like to get this in master before I need to do another rebase so if anyone has any objections just let me know and I'll try to turn them around ASAP.

As I mentioned in my previous attempt #267 the autocomplete is broken but that is not because of what I've done. That seems to be broken on master and I would like to tackle that after this gets merged if at all possible 😄

@DanChadwick
Copy link
Contributor

See the CONTRIBUTING.md for running jscs locally. The failures I see are style issues. That guide also has some info that you may or may not already know about how we are trying to do component ports to 1.0.

@mansona
Copy link
Collaborator Author

mansona commented Apr 22, 2016

thanks @DanChadwick 👍

Re: component ports, I thought I followed a previous guilde (or maybe it was copying early implementaitons) but is there something particular (Stylesheets, Naming, Attributes etc.) that you think I should re-look at?

@miguelcobain
Copy link
Collaborator

jscs should be running as part of the test suite since we've upgraded to ember-cli 2.4

@DanChadwick
Copy link
Contributor

I didn't have anything specific in contributing. I just thought it would be helpful. In fact, maybe you have things you think should be added it it??

@DanChadwick
Copy link
Contributor

@miguelcobain - I don't seem to see identical behavior between running tests locally and on travis. I can't recall what, but I remember seeing jscs "errors" caught by atom that weren't caught by ember test. I'll pay attention the next time I notice that.

@mansona
Copy link
Collaborator Author

mansona commented Apr 22, 2016

@DanChadwick ah cool, I thought you saw something horrific in my "style" and you were trying to be polite 😂

Looks like the tests are passing now. Let me know if there is anything I still need to change before we can merge 👍

@@ -217,7 +218,7 @@ export default Ember.Component.extend(HasBlockMixin, {
shouldHide: Ember.computed.not('isMinLengthMet'),

isMinLengthMet: Ember.computed('searchText', 'minLength', function() {
return this.get('searchText').length >= this.get('minLength');
return this.get('searchText.length') >= this.get('minLength');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this fix a bug because searchText is sometimes not a string? We are trying to isolate changes to autocomplete to minimize merge conflicts, since there is already WIP in another PR.

@DanChadwick
Copy link
Contributor

I made a few comments in anticipation of an eventual review by @miguelcobain. I would love to see this merged when ready. It doesn't seem to be as complete as the Angular implementation -- or at least the demo isn't as extensive -- but it seems to me to set a good baseline for basic use. There are other components which maybe are ahead of it in the queue. For example, tabs is currently in progress, and we would very much like to have that working so that we can use tabs for documentation purposes in the demo app.

Are you planning on tackling autocomplete after this?

@mansona mansona force-pushed the feature/new-new-chips branch from a0cfad6 to 630fad7 Compare June 14, 2016 12:06
@mansona
Copy link
Collaborator Author

mansona commented Jun 14, 2016

Ok so I've made most of the changes that you suggested @DanChadwick and reverted the stuff that you pointed out re: autocomplete.

I am still in a bit of bind here... the autocomplete implementation doesn't work but this is most likely because autocomplete is current broken in this branch. How far down the line is the new autocomplete work? I could really do with a modernised autocomplete implementation to finish off this PR.

Also what is your recommendation @miguelcobain re @DanChadwick 's question about the md-close icons #344 (comment)

@mansona
Copy link
Collaborator Author

mansona commented Jul 28, 2016

Hey Folks, I know you're all probably super busy. I am just checking if anyone has had a chance to look into this PR? Is there anything I can do to move it along or help out?

@pauln
Copy link
Contributor

pauln commented Sep 15, 2016

I'm also happy to help progress this PR - please let me know if there's anything I can do to get this moving along!

@xomaczar
Copy link
Contributor

There are a lot of work related to paper-autocomplete/paper-select/paper-menu happening right now on paper-menu branch. Hopefully very soon we'll have a stable version of paper-autocomplete merged into the master that should help finalize paper-chips impl.

@pauln
Copy link
Contributor

pauln commented Sep 21, 2016

I've rebased the feature/new-new-chips branch onto the current (as of yesterday) paper-menu branch, made it work with the paper-autocomplete from that branch (based on ember-power-select) and added some more polish and missing functionality (such as keyboard navigation). I'll hold off submitting a new (new new) PR until the paper-menu stuff lands and I've done a final rebase on that - I see there have already been more commits since I rebased; for now, feel free to take a look at https://github.com/pauln/ember-paper/tree/eps-chips if you want to see how it's progressing.

@mansona
Copy link
Collaborator Author

mansona commented Sep 21, 2016

@pauln i would recommend submitting your PR now as a work in progress, that way we can close this one 👍

@miguelcobain
Copy link
Collaborator

@pauln you can make a PR to the paper-menu branch and then we could close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants